Skip to content

set _api_write_item_iri only if result object is a subclass of a resource class#2810

Closed
Perf wants to merge 3 commits into
api-platform:2.4from
Perf:2.4
Closed

set _api_write_item_iri only if result object is a subclass of a resource class#2810
Perf wants to merge 3 commits into
api-platform:2.4from
Perf:2.4

Conversation

@Perf

@Perf Perf commented May 20, 2019

Copy link
Copy Markdown
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #2803
License MIT
Doc PR -

Issue happens when output class for a specific operation is not related to a class that represents api resource.
WriteListener tries to set _api_write_item_iri from a result object (defined in output attribute of an operation) but fails as it's not possible.

@maks-rafalko

Copy link
Copy Markdown
Contributor

Could you please use descriptive commit messages and update the title of this issue so it can be searched/indexed properly in the future? Thanks! :)

@teohhanhui

Copy link
Copy Markdown
Contributor

The fix is wrong. getIriFromResourceClass gives the collection IRI, not the item IRI.

I think we should just check whether it's a resource item, and skip setting the attribute if it's not. Because we should not make assumptions that won't hold true.

@teohhanhui

Copy link
Copy Markdown
Contributor

If you don't mind, maybe we could do this after #2797 is merged? I think it'll be easier.

@Perf Perf changed the title fix for #2803 set _api_write_item_iri only if result object is a subclass of a resource class May 20, 2019
@Perf

Perf commented May 20, 2019

Copy link
Copy Markdown
Author

Could you please use descriptive commit messages and update the title of this issue so it can be searched/indexed properly in the future? Thanks! :)

Hi @borNfreee,
thanks! Title and description updated.

@Perf

Perf commented May 20, 2019

Copy link
Copy Markdown
Author

The fix is wrong. getIriFromResourceClass gives the collection IRI, not the item IRI.

I think we should just check whether it's a resource item, and skip setting the attribute if it's not. Because we should not make assumptions that won't hold true.

Hi @teohhanhui,
true, I updated PR, no it will set _api_write_item_iri only if result object is a subclass of a resource class.

If you don't mind, maybe we could do this after #2797 is merged? I think it'll be easier.

You know better, I guess ;)

@olivermack

Copy link
Copy Markdown

Wouldn't it be less restrictive to check if the $controllerResult is just any kind of known ApiResource? With the current state of the PR we'd not be able to link to any other resource type other than the one that has been requested.

E.g. when we dispatch a request for a DTO or any other unidentifiable resource and the DataPersister returns any other applicable ApiResource than given in $attributes['resource_class'] the link may be especially handy - because a client simply could not even assume the result-link.

// ... SNIP
if ($hasOutput) {
    try {
        $request->attributes->set('_api_write_item_iri', $this->iriConverter->getIriFromItem($controllerResult));
    } catch (ResourceClassNotFoundException $e) {
        // swallow
    }
}

Unfortunately the IriConverter bubbles only a simple ApiPlatform\Core\Exception\InvalidArgumentException instead of a (possibly) more suitable ApiPlatform\Core\Exception\ResourceClassNotFoundException.

@karser

karser commented Jun 14, 2019

Copy link
Copy Markdown
Contributor

Looks like this issue is related #2860

@teohhanhui

Copy link
Copy Markdown
Contributor

See #2860 (comment)

I think this is not a bug.

@teohhanhui

Copy link
Copy Markdown
Contributor

Replaced by #2910

@teohhanhui teohhanhui closed this Jul 8, 2019
@teohhanhui teohhanhui self-assigned this Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants